-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fullbleed In App Messages #1018
Conversation
14ab8d5
to
6c82f9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @emawby, @Jeasmine, and @nan-li)
iOS_SDK/OneSignalSDK/Source/OSInAppMessageView.m, line 94 at r1 (raw file):
NSLog(@"11111 [self.webView loadHTMLString:html baseURL:url];"); NSLog(@"222222 [self.webView loadHTMLString:html baseURL:url];");
Remove NSLog
, or switch to OneSignal logger.
iOS_SDK/OneSignalSDK/Source/OSInAppMessageView.m, line 140 at r1 (raw file):
CGRect mainBounds = UIScreen.mainScreen.bounds;
These 5 new lines are duplicated in setIsFullscreen:
. We should deduplicate this code.
iOS_SDK/OneSignalSDK/Source/OSInAppMessageView.m, line 168 at r1 (raw file):
- (void)updateSafeAreaInsets { if (@available(iOS 11, *)) {
Prefer early return over long if block.
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 275 at r1 (raw file):
} - (NSString *)setContentInsetsInHTML:(NSString *)html {
This code looks very similar to updateSafeAreaInsets
in OSInAppMessageView
. Is there a reason for the difference or could they be made the same?
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 280 at r1 (raw file):
UIWindow *keyWindow = UIApplication.sharedApplication.keyWindow; if (!keyWindow) { return newHTML;
We can't support fullscreen if there isn't a keyWindow
? Is there another way to support apps that doesn't have this?
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 736 at r1 (raw file):
} case OSInAppMessageBridgeEventTypePageResize: { // Unused resize event for IAM during actions like orientation changes and displaying an IAM
Update comment, this resize is used now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Jeasmine, @jkasten2, and @nan-li)
iOS_SDK/OneSignalSDK/Source/OSInAppMessageView.m, line 94 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Remove
NSLog
, or switch to OneSignal logger.
Done.
iOS_SDK/OneSignalSDK/Source/OSInAppMessageView.m, line 140 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
These 5 new lines are duplicated in
setIsFullscreen:
. We should deduplicate this code.
Done.
iOS_SDK/OneSignalSDK/Source/OSInAppMessageView.m, line 168 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Prefer early return over long if block.
Xcode warnings are not smart enough for that unfortunately. It will complain that you aren't guarding availability. I would rather not use compiler flags to remove the warning
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 275 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
This code looks very similar to
updateSafeAreaInsets
inOSInAppMessageView
. Is there a reason for the difference or could they be made the same?
updateSafeAreaInsets is using the evaluateJavaScript method on a webView whereas setContentInsetsInHTML is modifying the HTML string.
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 280 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
We can't support fullscreen if there isn't a
keyWindow
? Is there another way to support apps that doesn't have this?
So the only way there wouldn't be a keyWindow is if something is going wrong and the IAM is being displayed before any UI has been rendered (this still works for SwiftUI) . The deprecation of keywindow is because it is possible for iOS apps to have multiple windowScenes and this will return a window across all connected scenes, but I think that is actually what we want since we only care about safe area insets. This check is only to avoid the potential rare crash.
If we cannot get a window then we cannot find out the safeAreaInsets so we cannot adjust the content.
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 736 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Update comment, this resize is used now.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine and @nan-li)
iOS_SDK/OneSignalSDK/Source/OSInAppMessageView.m, line 168 at r1 (raw file):
Previously, emawby (Elliot Mawby) wrote…
Xcode warnings are not smart enough for that unfortunately. It will complain that you aren't guarding availability. I would rather not use compiler flags to remove the warning
👍
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 275 at r1 (raw file):
Previously, emawby (Elliot Mawby) wrote…
updateSafeAreaInsets is using the evaluateJavaScript method on a webView whereas setContentInsetsInHTML is modifying the HTML string.
👍
iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 280 at r1 (raw file):
Previously, emawby (Elliot Mawby) wrote…
So the only way there wouldn't be a keyWindow is if something is going wrong and the IAM is being displayed before any UI has been rendered (this still works for SwiftUI) . The deprecation of keywindow is because it is possible for iOS apps to have multiple windowScenes and this will return a window across all connected scenes, but I think that is actually what we want since we only care about safe area insets. This check is only to avoid the potential rare crash.
If we cannot get a window then we cannot find out the safeAreaInsets so we cannot adjust the content.
👍
Description
One Line Summary
Added support for fullbleed in app messages to the iOS SDK.
Details
Fullscreen and Carousel In App Messages that do not have margins will now bleed under the status bar to be completely full screen.
The dashboard has added the ability for fullscreen and Carousel IAMs to be created without margins. When doing so it sets
remove_height_margin
andremove_width_margin
to true instyles
(this was already being read by the SDK). Currently we don't support only width or height removal so I am consolidating these flags into an isFullscreen boolean that controls the fullbleed behavior.In order to handle notched devices we need to tell the Javascript about the device insets so that IAM content doesn't overlap with notches and the status bar. To do this we are modifying the IAMs HTML and calling setSafeAreaInsets with a javascript object of the form
Orientation Change
We also need to call the setSafeAreaInsets() method again if the device is rotated since the side of the notches have changed. To do so we are now responding to the resize javascript event (previously unused).
Misc Changes
Since we need to access UIApplications's keyWindow safeAreaInsets we need to be on the main thread once we receive the message's HTML. I have changed where we go to the main thread from the message view to the response of the loadHTML call.
Additionally the webView needs to know if it is fullScreen right away so that carousels are provided with the correct width on initial load and orientation change. To do this I added a setIsFullscreen method to
OSInAppMessageView
that enables/disables margins.Motivation
Customer request for full screen IAMs.
Background Context
When we initially discussed removing margins from IAMs we discussed the future ability to remove just width or height margins so we split them into their own booleans. We eventually decided against that approach and to just focus on fullbleed IAMs.
Scope
This change involves coordinating new behavior between the SDK and IAM Javascript. It makes no new API calls and provides no new public APIs in the SDK. This change should not affect non fullscreen/carousel IAMs.
Testing
Unit testing
✅ All tests pass after the below modifications
We don't infrastructure for testing properties of the OSInAppMessageViewController currently or good ways to test a messages HTML content.
Manual testing
✅ Tested on various iOS simulators for carousels and fullscreen IAMs. Tested both landscape and portrait
Example of the Result
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is